Remove unused Promise polyfill and promise dependency - #57777
Open
aravi365 wants to merge 1 commit into
Open
Conversation
Every supported JS engine ships a native Promise, but the polyfill branch in polyfillPromise.js is statically reachable, so Metro bundles the promise package into every app even though it is never used. Remove the dead branch, Libraries/Promise.js and the promise dependency. DEV unhandled-rejection tracking is unchanged. Fixes react#57702
meta-cla
Bot
added
the
CLA Signed
facebook-github-tools
Bot
added
the
Shared with Meta
|
@robhogan has imported this pull request. If you are a Meta employee, you can view this in D114354694. |
Contributor
Author
|
@robhogan you imported this on 31 Jul and both internal checks have been red since. I can't see the internal logs from out here, so I'm guessing: is something in the internal codebase still importing Happy to narrow this if it helps. I can keep the polyfill and gate it so only non-Hermes engines load it, which leaves any internal call sites working while still keeping Let me know which you'd prefer and I'll push it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #57702.
polyfillPromise.jsrequires../PromisewhenHermesInternal.hasPromise()is falsy. Metro can't know that condition at build time, so it bundles the require anyway and thepromisepackage ends up in every app bundle even though Hermes apps never reach that branch.This drops the fallback branch,
Libraries/Promise.js, thepromisedependency and its flow-typed def.DEV unhandled-rejection tracking is unaffected, that goes through
HermesInternal.enablePromiseRejectionTracker.promiseRejectionTrackingOptions.jswas importing its options type frompromise/setimmediate/rejection-tracking, so I've declared that type inline.Open question: this also drops the fallback for non-Hermes engines (JSC, V8). Both have had a native Promise for years and the "Event loop not supported" case the fallback was written for is long gone, but it is still a behavior change, so your call.
Both internal checks are red and I can't see the logs from out here. If something internal still imports
Libraries/Promiseorpromisedirectly, I can cut this down to just deleting theelsebranch and leave the file and the dependency in place. That still keepspromiseout of the bundle, which is what the issue is about. Say the word and I'll push it.Changelog:
[GENERAL] [REMOVED] - Remove the unused Promise polyfill and the promise dependency
Test Plan
Libraries/Promiseorpromise/setimmediateare left anywhere in the repo.promiseline is removed frompackage.json.hasPromise()is true there, soglobal.Promisewas already what apps used, and the DEV rejection tracker still gets the same options object.I have not run the JS suite locally, relying on CI for
test_js, Flow and lint.